Skip to content

Add utility function mapSourceRepos #93

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 8 commits into from
Jun 14, 2018
Merged

Add utility function mapSourceRepos #93

merged 8 commits into from
Jun 14, 2018

Conversation

jneira
Copy link
Collaborator

@jneira jneira commented Jun 12, 2018

As suggested by @ocharles in #73 is the best and generic way to modify the fields of source repos. As source-repos is a list i've chosen to map over the complete list.
I've add a test case for the new function and remove some trailing spaces in some files.

No directly related with this but, what do you think about move the utility functions to a subfoler utils and import them in prelude like types or defaults:

utils = { majorVersions = ./utils/MajorVersions.dhall
          , Github-project = ./utils/Github-project.dhall
          , mapSourceRepos = ./utils/mapSourceRepos.dhall
         }

ocharles
ocharles reviewed Jun 12, 2018
View reviewed changes
golden-tests/dhall-to-cabal/map-source-repo.cabal
@@ -0,0 +1,17 @@
name: repo
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think what we actually really want is a test that mapSourceRepo produces some normalized Dhall, but it might be a little out of scope for this PR. If you want to add a new section to the golden tests (that basically golden test one Dhall file normalizes to another), I'd be on board with that. If not, it can be done later.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That section would be usefult to test other prelude functions in isolation (among other things), right?
I agree it would be usefult but if you dont mind i would prefer to add it in another pr.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another PR is good with me!

@ocharles ocharles mentioned this pull request Jun 12, 2018
Merged
@jneira
Copy link
Collaborator Author

jneira commented Jun 12, 2018

If you agree with moving the utility functions i could add it in this one (or in another one).

@ocharles
Copy link
Member

@jneira what do you mean by "moving the utility functions"? I'm ok with merging this PR and then having a new PR that adds testing to prelude functions.

@jneira
Copy link
Collaborator Author

jneira commented Jun 12, 2018

@ocharles I mean moving the prelude.utils functions (Github-project.dhall and mapSourceRepos.dhall) to a subfolder utils (including majorVersions in its own file) as i commented above. It seems more utils functions will be added in the near future....
But maybe better in other pr too.

@ocharles
Copy link
Member

ocharles commented Jun 12, 2018 •
edited
Loading

How about copying the implicit structure being set out by https://github.com/dhall-lang/Prelude? That would mean that each folder would have a package.dhall, so if you wanted just utilities, you could import utils/package.dhall. Is that along the lines of what you're thinking? Then prelude.dhall would have { utils = ./utils/package.dhall }. I think I understand your comment now, and if you want to do it in the aforementioned fashion, I'm good with that.

TL,DR: I'm good with having utils/package.dhall, ./utils/MajorVersions.dhall, etc, and changing prelude.dhall to have { utils = ./utils/package.dhall }

@jneira
Copy link
Collaborator Author

jneira commented Jun 13, 2018

@ocharles much better with the package file which, moreover, follows existing conventions, thanks for the point

ocharles
ocharles requested changes Jun 13, 2018
View reviewed changes
dhall-to-cabal.cabal Outdated
-- Instead, edit the source Dhall file (which may have the
-- '.dhall' extension) and re-run dhall-to-cabal, passing the
-- source file's name as its argument and redirecting output to
-- 'dhall-to-cabal.cabal' (this file).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One last thing, could you regenerate this file with the instructions given? That is:

dhall-to-cabal -- dhall-to-cabal.dhall > dhall-to-cabal.cabal

That will keep this warning the same, and it's slightly more instructive.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mmm, i did recreate the file using dhall-to-cabal itself (built with master version). I think it is caused by building the executable in windows, cause win paths doesnt satisfy the isAcceptableCommentChar check.
Moreover the suggestion is only valid in *nix envs (should be type dhall-to-cabal.dhall | dhall-to-cabal > dhall-to-cabal.cabal for win)
But whatever, i guess there are not much dhall-to-cabal users in windows but me for now 😉
so i will update the message (make it suitable for window would be in another pr)

@jneira
Copy link
Collaborator Author

jneira commented Jun 14, 2018

@ocharles do you think it would need some other change?

@ocharles ocharles merged commit 18db76b into dhall-lang:master Jun 14, 2018
@ocharles
Copy link
Member

Perfect, thank you!

@jneira jneira deleted the map-src-repos branch June 14, 2018 21:29
@jneira jneira mentioned this pull request Jun 19, 2018
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ocharles ocharles ocharles approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jneira @ocharles